rdbms: clean up non-native Database::replace() code

* Make sure all unique keys specified have all their values
  provided to avoid large bogus DELETEs. Do not ignore them
  in such cases either, as that would cause inconsistencies
  between the native and non-native case. Use an exception.
* Make ChangeTags caller clearer that the list of indexes
  is not a list of fields for a single index. Also, avoid
  mentioning indexes for values not defined in the new
  records, as this causes errors or inconsistencies with
  the native vs non-native case.
* This also fixes the "Undefined index: ts_log_id" error
  when running unit tests on postgres.

Change-Id: I30263df22066bd6d4836202b1bcad5d1aa1e7383

diff --git a/includes/changetags/ChangeTags.php b/includes/changetags/ChangeTags.php
index 95848ea..7e4dd00 100644 (file)
--- a/includes/changetags/ChangeTags.php
+++ b/includes/changetags/ChangeTags.php
@@ -408,19 +408,24 @@ class ChangeTags {
                sort( $prevTags );
                sort( $newTags );
                if ( $prevTags == $newTags ) {
-                       // No change.
                        return false;
                }
 
                if ( !$newTags ) {
-                       // no tags left, so delete the row altogether
+                       // No tags left, so delete the row altogether
                        $dbw->delete( 'tag_summary', $tsConds, __METHOD__ );
                } else {
-                       $dbw->replace( 'tag_summary',
-                               [ 'ts_rev_id', 'ts_rc_id', 'ts_log_id' ],
-                               array_filter( array_merge( $tsConds, [ 'ts_tags' => implode( ',', $newTags ) ] ) ),
-                               __METHOD__
-                       );
+                       // Specify the non-DEFAULT value columns in the INSERT/REPLACE clause
+                       $row = array_filter( [ 'ts_tags' => implode( ',', $newTags ) ] + $tsConds );
+                       // Check the unique keys for conflicts, ignoring any NULL *_id values
+                       $uniqueKeys = [];
+                       foreach ( [ 'ts_rev_id', 'ts_rc_id', 'ts_log_id' ] as $uniqueColumn ) {
+                               if ( isset( $row[$uniqueColumn] ) ) {
+                                       $uniqueKeys[] = [ $uniqueColumn ];
+                               }
+                       }
+
+                       $dbw->replace( 'tag_summary', $uniqueKeys, $row, __METHOD__ );
                }
 
                return true;

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index 1efa9a1..6d1cff7 100644 (file)
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -2238,49 +2238,43 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
        }
 
        public function replace( $table, $uniqueIndexes, $rows, $fname = __METHOD__ ) {
-               $quotedTable = $this->tableName( $table );
-
                if ( count( $rows ) == 0 ) {
                        return;
                }
 
-               # Single row case
+               // Single row case
                if ( !is_array( reset( $rows ) ) ) {
                        $rows = [ $rows ];
                }
 
-               // @FXIME: this is not atomic, but a trx would break affectedRows()
+               // @FIXME: this is not atomic, but a trx would break affectedRows()
                foreach ( $rows as $row ) {
-                       # Delete rows which collide
-                       if ( $uniqueIndexes ) {
-                               $sql = "DELETE FROM $quotedTable WHERE ";
-                               $first = true;
-                               foreach ( $uniqueIndexes as $index ) {
-                                       if ( $first ) {
-                                               $first = false;
-                                               $sql .= '( ';
-                                       } else {
-                                               $sql .= ' ) OR ( ';
-                                       }
-                                       if ( is_array( $index ) ) {
-                                               $first2 = true;
-                                               foreach ( $index as $col ) {
-                                                       if ( $first2 ) {
-                                                               $first2 = false;
-                                                       } else {
-                                                               $sql .= ' AND ';
-                                                       }
-                                                       $sql .= $col . '=' . $this->addQuotes( $row[$col] );
-                                               }
-                                       } else {
-                                               $sql .= $index . '=' . $this->addQuotes( $row[$index] );
-                                       }
+                       // Delete rows which collide with this one
+                       $indexWhereClauses = [];
+                       foreach ( $uniqueIndexes as $index ) {
+                               $indexColumns = (array)$index;
+                               $indexRowValues = array_intersect_key( $row, array_flip( $indexColumns ) );
+                               if ( count( $indexRowValues ) != count( $indexColumns ) ) {
+                                       throw new DBUnexpectedError(
+                                               $this,
+                                               'New record does not provide all values for unique key (' .
+                                                       implode( ', ', $indexColumns ) . ')'
+                                       );
+                               } elseif ( in_array( null, $indexRowValues, true ) ) {
+                                       throw new DBUnexpectedError(
+                                               $this,
+                                               'New record has a null value for unique key (' .
+                                                       implode( ', ', $indexColumns ) . ')'
+                                       );
                                }
-                               $sql .= ' )';
-                               $this->query( $sql, $fname );
+                               $indexWhereClauses[] = $this->makeList( $indexRowValues, LIST_AND );
+                       }
+
+                       if ( $indexWhereClauses ) {
+                               $this->delete( $table, $this->makeList( $indexWhereClauses, LIST_OR ), $fname );
                        }
 
-                       # Now insert the row
+                       // Now insert the row
                        $this->insert( $table, $row, $fname );
                }
        }

diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
index 23f7865..d8ebeff 100644 (file)
--- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
+++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
@@ -560,7 +560,7 @@ class DatabaseSQLTest extends PHPUnit_Framework_TestCase {
                                        'rows' => [ 'field' => 'text', 'field2' => 'text2' ],
                                ],
                                "DELETE FROM replace_table " .
-                                       "WHERE ( field='text' ); " .
+                                       "WHERE (field = 'text'); " .
                                        "INSERT INTO replace_table " .
                                        "(field,field2) " .
                                        "VALUES ('text','text2')"
@@ -576,7 +576,7 @@ class DatabaseSQLTest extends PHPUnit_Framework_TestCase {
                                        ],
                                ],
                                "DELETE FROM module_deps " .
-                                       "WHERE ( md_module='module' AND md_skin='skin' ); " .
+                                       "WHERE (md_module = 'module' AND md_skin = 'skin'); " .
                                        "INSERT INTO module_deps " .
                                        "(md_module,md_skin,md_deps) " .
                                        "VALUES ('module','skin','deps')"
@@ -598,12 +598,12 @@ class DatabaseSQLTest extends PHPUnit_Framework_TestCase {
                                        ],
                                ],
                                "DELETE FROM module_deps " .
-                                       "WHERE ( md_module='module' AND md_skin='skin' ); " .
+                                       "WHERE (md_module = 'module' AND md_skin = 'skin'); " .
                                        "INSERT INTO module_deps " .
                                        "(md_module,md_skin,md_deps) " .
                                        "VALUES ('module','skin','deps'); " .
                                        "DELETE FROM module_deps " .
-                                       "WHERE ( md_module='module2' AND md_skin='skin2' ); " .
+                                       "WHERE (md_module = 'module2' AND md_skin = 'skin2'); " .
                                        "INSERT INTO module_deps " .
                                        "(md_module,md_skin,md_deps) " .
                                        "VALUES ('module2','skin2','deps2')"
@@ -625,12 +625,12 @@ class DatabaseSQLTest extends PHPUnit_Framework_TestCase {
                                        ],
                                ],
                                "DELETE FROM module_deps " .
-                                       "WHERE ( md_module='module' ) OR ( md_skin='skin' ); " .
+                                       "WHERE (md_module = 'module') OR (md_skin = 'skin'); " .
                                        "INSERT INTO module_deps " .
                                        "(md_module,md_skin,md_deps) " .
                                        "VALUES ('module','skin','deps'); " .
                                        "DELETE FROM module_deps " .
-                                       "WHERE ( md_module='module2' ) OR ( md_skin='skin2' ); " .
+                                       "WHERE (md_module = 'module2') OR (md_skin = 'skin2'); " .
                                        "INSERT INTO module_deps " .
                                        "(md_module,md_skin,md_deps) " .
                                        "VALUES ('module2','skin2','deps2')"